Skip to content

Enable strictObjectIDCoercion for RoleMapping model#437

Merged
bajtos merged 1 commit into
masterfrom
fix/enable-strictObjectIDCoercion-for-RoleMapping
Feb 22, 2017
Merged

Enable strictObjectIDCoercion for RoleMapping model#437
bajtos merged 1 commit into
masterfrom
fix/enable-strictObjectIDCoercion-for-RoleMapping

Conversation

@bajtos

@bajtos bajtos commented Feb 17, 2017

Copy link
Copy Markdown
Member

Description

Force the RoleMapping model to always store ids as ObjectId when using MongoDB connector. This is needed to allow users to query RoleMapping via "principalId" property.

The solution uses a less-known feature where server/model-config.json can provide additional "options" to apply even to built-in models like RoleMapping.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

Force the RoleMapping model to always store ids as ObjectId when using
MongoDB connector. This is needed to allow users to query RoleMapping
via "principalId" property.
@bajtos
bajtos force-pushed the fix/enable-strictObjectIDCoercion-for-RoleMapping branch from 062d395 to bfb115f Compare February 17, 2017 12:29
@bajtos

bajtos commented Feb 22, 2017

Copy link
Copy Markdown
Member Author

@slnode test please

@bajtos
bajtos merged commit 75aefef into master Feb 22, 2017
@bajtos
bajtos deleted the fix/enable-strictObjectIDCoercion-for-RoleMapping branch February 22, 2017 13:52
@bajtos bajtos removed the review label Feb 22, 2017
@bajtos

bajtos commented Feb 22, 2017

Copy link
Copy Markdown
Member Author

Released in loopback-workspace@3.38.0.

@ebarault

ebarault commented Apr 6, 2017

Copy link
Copy Markdown

@bajtos : i just helped someone else for whom ACLs were not working because RoleMapping had principalId as string, not ObjectId. This bug is painful.

Remind me please: why can't we just add this option to the built-in RoleMapping model in loopback core (or do the config in app.js at app boot if a feature flag is required) ?

"options": {
  "strictObjectIDCoercion": true
},

I don't see anything in the PR that actually tests if the connector is mongo before applying this option to model-config.json so i assume it does not harm when using other connectors

@bajtos

bajtos commented Apr 7, 2017

Copy link
Copy Markdown
Member Author

@ebarault

Remind me please: why can't we just add this option to the built-in RoleMapping model in loopback core

Backwards compatibility, as usually. See strongloop/loopback#3198 (comment)

strictObjectIDCoercion will also dictate how ID's are written to mongo in the first place. In other words, it can only be enabled for 'fresh' projects, or after a db migration of legacy (string) ID's in existing applications.

You are welcome :)


(or do the config in app.js at app boot if a feature flag is required) ?

I think that's what this pull request is effectively doing in new applications - setting the required feature flag the way how it can/should be set.

Existing applications can apply the same approach - simply modify the RoleMapping section in server/model-config.json to the following:

{
  "RoleMapping": {
    "dataSource": "db",
    "options": {
      "strictObjectIDCoercion": true,
    },
    "public": false
  }
}

I do realise the current situation is unfortunate and I am happy to discuss other/better ways how to solve this problem.

I think ideally we should fix the root cause, which IIRC lies in the way how foreign keys are treated by LoopBack - the type of foreign keys should be changed from any to number, string, or ObjectID depending on what is the database the target model is attached to.

@superkhau was already taking a stab at that issue, but with little success. You can search for "fix ObjectID coercion", here are few issues to get started:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants